Added aio.h implementations for FreeBSD, NetBSD and DragonFly by dkgroot · Pull Request #2133 · dlang/druntime

dkgroot · 2018-03-06T15:25:57Z

Consists of two seperate commits:

Fix the previous implementation (add aio.d for linux x64 #2074).
Note: ssize_t aio_return(const(aiocb)* aiocbp); should have been ssize_t aio_return(aiocb* aiocbp); (According to Linux, BSD's and OpenGroup-Posix )
Add implementations for FreeBSD, NetBSD and DragonFly

Port Requested by @ibuclaw : #2074 (comment)
Ping @Darredevil

dlang-bot · 2018-03-06T15:25:58Z

Thanks for your pull request and interest in making D better, @dkgroot! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2133"

dkgroot · 2018-03-06T16:18:27Z

@ibuclaw As per your request: #2074 (comment)

ibuclaw · 2018-03-06T16:51:35Z

src/core/sys/posix/aio.d

+        struct __aiocb_private {
+            long status;
+            long error;
+            void *kernelinfo;


Should probably use D style type here, and in other places (void* foo, not void *foo).

Q:Would it make sense to to extend checkwhitespace.d to also check for this ?

We should have a much more comprehensive lint used for phobos, so we should probably replace checkwhitespace with that and spend effort on something that may be replaced in the future.

ibuclaw · 2018-03-06T16:53:56Z

src/core/sys/posix/aio.d

+            private ssize_t _retval;
+        }
+
+        version = bsd_posix;


Not sure about this, does it really make things easier to have this instead of specific versions?

Don't mind either way.

Given how small this module is, I mean.

I think it focusses the attention on the difference between the enums' ordering (I was actually a little surprised that open groups posix specification does not state the values for these constants :-))

Would be happy to change it though, let's see if somebody would like it changed as well.

Can we proceed with removing version = bsd_posix? Just to speed up pulling this in, as this is the only part I'm hesitant about here.

dkgroot · 2018-03-07T11:36:59Z

BTW: Would it be helpfull to copy the comments/documentation over from the linux/bsd header files. I noticed that some of the other sys/posix/*.d files did have this ?

PetarKirov · 2018-03-09T09:19:15Z

@dkgroot AFAIK we avoid copying comments since they are covered by copyright, unlike pure API bindings.

PetarKirov

LGTM, modulo a few style nits.

PetarKirov · 2018-03-09T09:24:16Z

src/core/sys/posix/aio.d

+    }
+    else version (FreeBSD)
+    {
+        struct __aiocb_private {


Put brace on next line.

PetarKirov · 2018-03-09T09:24:36Z

src/core/sys/posix/aio.d

+        LIO_READ,
+        LIO_WRITE,
+        LIO_NOP
+    };


Remove semicolon.

PetarKirov · 2018-03-09T09:25:41Z

src/core/sys/posix/aio.d

+/* functions outside/extending posix requirement */
+version (FreeBSD)
+{
+    int aio_waitcomplete(aiocb ** aiocb_list, const(timespec)* timeout);


dkgroot · 2018-03-19T22:09:42Z

Ping @MartinNowak @CyberShadow

dkgroot · 2018-04-11T15:16:38Z

Ping @MartinNowak @CyberShadow

CyberShadow · 2018-04-11T15:36:09Z

I don't use these operating systems so I can't really vouch for this, but the diff looks good.

Would you mind squashing the changes from the last commit into the first two?

dkgroot · 2018-04-11T15:57:39Z

@CyberShadow Thanks for checking.
Don't mind squashing, but wanted to preserve the two first commits to show the distinction (The first one is fixing the previously merged implementation, the second is extending it by adding the BSD implementation). The squashing can be done when someone has been able to review it.

Do you know anybody who does use one of the BSD's for checking/testing ? If so can you add them as a reviewer ?

CyberShadow · 2018-04-11T16:22:55Z

Don't mind squashing, but wanted to preserve the two first commits to show the distinction

Yep, my suggestion was to just get rid of the third commit, and leave the first two separate.

Do you know anybody who does use one of the BSD's for checking/testing ? If so can you add them as a reviewer ?

Other than the people already in this thread, I don't think so; so, we might as well merge this after the fixup.

…nter to aiocbp.

dkgroot · 2018-05-02T12:08:45Z

Does one of you know why auto-tester fails on linux_64_32 for this change, i don't see how 'std/regex/internal/tests.d' could be related to these changes. Maybe it's just an unrelated regression.

wilzbach · 2018-05-02T12:39:25Z

Spurious failure because std.regex's CTFE tests consumes so much memory (see e.g. dlang/phobos#6164)
auto-tester automatically restarts itself.

dkgroot · 2018-05-02T14:55:02Z

@wilzbach Thanks for the heads-up !
Ping @MartinNowak
Anything holding this PR up ?

wilzbach · 2018-05-06T01:42:24Z

version (X86_64)
{
...
}
else version (DragonFlyBSD)
{

}


I thought DragonFlyBSD is x86_64 only?

dkgroot · 2018-05-06T10:06:28Z

@wilzbach Not sure how this came to be (It has been a couple of weeks). It looks like the BSD versions are at the wrong version scope (indentation). Thanks for noticing/finding this !

ibuclaw

Let's just proceed. As the incomplete implementation (unsupported cpu type is just plain wrong in the GNU path) is blocking me.

bsd_posix should perhaps by BSD_Posix?

dkgroot requested review from CyberShadow and MartinNowak as code owners March 6, 2018 15:25

dkgroot force-pushed the port_aio branch 2 times, most recently from d0e6b37 to 3ab765b Compare March 6, 2018 16:07

ibuclaw reviewed Mar 6, 2018

View reviewed changes

dkgroot force-pushed the port_aio branch 2 times, most recently from 1ee0732 to dee866d Compare March 8, 2018 11:52

PetarKirov reviewed Mar 9, 2018

View reviewed changes

dkgroot force-pushed the port_aio branch from d7b943c to 08196c1 Compare April 11, 2018 19:26

Fix aio.d glibc implementation, aio_return() does not use a const poi…

9c2f494

…nter to aiocbp.

dkgroot force-pushed the port_aio branch from 08196c1 to 2c88dfd Compare May 2, 2018 10:39

dkgroot force-pushed the port_aio branch 2 times, most recently from 59c967e to c10c6d0 Compare May 6, 2018 10:15

Added aio.h implementations for FreeBSD, NetBSD and DragonFly

8edf155

dkgroot force-pushed the port_aio branch from c10c6d0 to 8edf155 Compare May 6, 2018 20:34

ibuclaw approved these changes Jun 23, 2018

View reviewed changes

ibuclaw added the auto-merge label Jun 23, 2018

dlang-bot merged commit c06ed31 into dlang:master Jun 23, 2018

Uh oh!

Conversation

dkgroot commented Mar 6, 2018

Uh oh!

dlang-bot commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

dkgroot commented Mar 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot commented Mar 7, 2018

Uh oh!

PetarKirov commented Mar 9, 2018

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot commented Mar 19, 2018

Uh oh!

dkgroot commented Apr 11, 2018

Uh oh!

CyberShadow commented Apr 11, 2018

Uh oh!

dkgroot commented Apr 11, 2018

Uh oh!

CyberShadow commented Apr 11, 2018

Uh oh!

dkgroot commented May 2, 2018

Uh oh!

wilzbach commented May 2, 2018

Uh oh!

dkgroot commented May 2, 2018

Uh oh!

wilzbach commented May 6, 2018

Uh oh!

dkgroot commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dlang-bot commented Mar 6, 2018 •

edited

Loading

dkgroot Mar 6, 2018 •

edited

Loading

dkgroot commented May 6, 2018 •

edited

Loading